Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

MCPForUnity/Editor/Tools/Animation/ControllerCreate.cs (2)

499-506: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard positions items before property indexing to avoid runtime exceptions.

On Line 501–506, the loop indexes token["..."] without verifying object shape. Non-object entries in positions can throw and convert client input errors into internal failures. Add an if (token is not JObject item) continue/collect-error guard before field access.

Minimal fix

 foreach (var token in positions)
 {
-    string name = token["name"]?.ToString();
+    if (token is not JObject item)
+        continue;
+
+    string name = item["name"]?.ToString();
     if (string.IsNullOrEmpty(name))
         continue;
-    float x = token["x"]?.ToObject<float>() ?? 0f;
-    float y = token["y"]?.ToObject<float>() ?? 0f;
-    int? layer = token["layer"]?.ToObject<int>() ?? defaultLayer;
+    float x = item["x"]?.ToObject<float>() ?? 0f;
+    float y = item["y"]?.ToObject<float>() ?? 0f;
+    int? layer = item["layer"]?.ToObject<int>() ?? defaultLayer;
     want[$"{(layer.HasValue ? layer.Value.ToString() : "*")}:{name}"] = new Vector2(x, y);
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@MCPForUnity/Editor/Tools/Animation/ControllerCreate.cs` around lines 499 -
506, In the foreach loop that iterates over positions, add a guard clause at the
beginning to verify that each token is a JObject before attempting to access its
properties using the indexer (e.g., token["name"]). If the token is not a
JObject, use continue to skip it or handle the error appropriately. This
prevents runtime exceptions when non-object entries exist in the positions
collection and converts potential client input errors into graceful handling
rather than internal failures.

---

497-507: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

State key is still ambiguous for duplicate names within the same layer.

On Line 497 and Line 541, matching is keyed by {layer}:{name} (or wildcard), which is not unique across sub-state-machines in that layer. A single request can still move multiple unintended nodes when names repeat. The read/write contract needs a unique identifier (e.g., layer + state-machine path + state name) in both GetStatePositions output and SetStatePositions matching.

Suggested direction

- // Returns [{ name, x, y, layer }]
+ // Returns [{ id, name, x, y, layer, path }]

- want[$"{(layer.HasValue ? layer.Value.ToString() : "*")}:{name}"] = new Vector2(x, y);
+ // Expect `id` from GetStatePositions; use that as the only write key.
+ want[id] = new Vector2(x, y);

- string scopedKey = $"{layer}:{name}";
- string anyKey = $"*:{name}";
+ string scopedKey = BuildStateId(layer, stateMachinePath, name);

Also applies to: 539-545

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@MCPForUnity/Editor/Tools/Animation/ControllerCreate.cs` around lines 497 -
507, The state positioning key format using only layer and name is ambiguous
when duplicate state names exist in different sub-state-machines within the same
layer. Update the key structure to include the state-machine path in addition to
layer and name to ensure uniqueness. In the section around line 497-507 where
the want dictionary is built, modify the key format from {layer}:{name} to
include the full state-machine path (e.g., {layer}:{stateMachinePath}:{name}).
Apply the same key format change in the matching logic around lines 539-545 to
ensure consistency between how positions are stored and retrieved. This requires
coordinating the GetStatePositions output format with the SetStatePositions
matching logic to use the same unique identifier scheme.

Server/tests/test_manage_animation.py (1)

75-82: ⚡ Quick win

Include controller_remove_transition in the expected controller action contract test.

CONTROLLER_ACTIONS now includes controller_remove_transition, but this expected-set assertion doesn’t check it, so allowlist regressions for that action can slip through.

Suggested test update

     def test_expected_controller_actions_present(self):
         expected = {"controller_create", "controller_add_state",
                     "controller_set_state_properties", "controller_get_state_properties",
-                    "controller_add_transition",
+                    "controller_add_transition", "controller_remove_transition",
                     "controller_add_parameter", "controller_get_info", "controller_assign",
                     "controller_add_layer", "controller_remove_layer", "controller_set_layer_weight",
                     "controller_create_blend_tree_1d", "controller_create_blend_tree_2d", "controller_add_blend_tree_child"}
         assert expected.issubset(set(CONTROLLER_ACTIONS))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Server/tests/test_manage_animation.py` around lines 75 - 82, The
test_expected_controller_actions_present method has an expected set of
controller actions but is missing controller_remove_transition, which is now
part of CONTROLLER_ACTIONS. Add controller_remove_transition to the expected set
in the assertion so that the test will catch if this action is accidentally
removed from CONTROLLER_ACTIONS in the future. Include it in the set of expected
action strings being checked against CONTROLLER_ACTIONS.